wallet: Privatize Core descriptor records - #64
BenWestgate wants to merge 16 commits into
Conversation
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review, posted at the maintainer's request.
ACK a0e3a15.
|
Review sequencing note: #64 remains the focused fix for #63's public API boundary and preserves the currently supported arbitrary |
|
Agent release-gate review at exact head |
808b6fd to
f3b17be
Compare
|
AI-assisted release-gate recheck of current head Code ACK. This refresh replays only the still-valid public-API cleanup onto current Focused wallet/public-API/Core tests pass 64/64 normally and 64/64 under No remaining code-review blocker found on this head; it is ready for human review. |
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review performed at the maintainer's request and disclosed per docs/developer/AI_POLICY.md.
ACK f3b17be. The refreshed one-commit diff cleanly removes the obsolete supported core_descriptors surface without touching the Core-native account-0 initialization introduced by #7. master_xprv remains public; the fixed descriptor builder is private verification tooling only; installed/public-API checks assert the removed symbol is no longer exported. The package __all__ count becomes 23 as intended. Exact-head Python-package and Bitcoin Core fixture workflows both pass. No correctness findings; ready for human review/integration after the runtime stack settles.
f3b17be to
43af20d
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Apply the established majority-case interpretation to standalone correction while preserving immutable context, entered edit semantics, and disclosure accounting. Account the normalized retry frontier even when the first optional search reaches its deadline after finding a candidate, so cumulative capture mass remains fail-closed. Normalize ordinary grouping spaces before locating an immutable prefix in the public API, so grouped input cannot shift the mixed-case boundary into a locked header. Report truncation from the shared mixed-case schedule: combine both full passes' completeness, and mark returned candidates search_complete=False when either pass truncated. The deadline regressions cover a string only the erasure reading corrects (five minority-case P) and one only case normalization corrects (fifteen minority-case X, one mistyped); both recover within ten seconds while the normalized exhaustive optional search may truncate. Fixes #37.
Give standalone correction a stable status contract: 0 for already-valid input, 1 when a suggestion is emitted, 2 for command or input syntax errors, and 3 when no usable suggestion is emitted. Keep incomplete best-effort suggestions at status 1 and document status 3 only for incomplete searches without a usable suggestion. Fixes #39.
Remove unreachable creation guards and the permanently false correction ambiguity field, align the CLI test Core stub with production, and move reference-only correction helpers out of the installed package. Security: fail-closed correction and wallet behavior are unchanged. Refs #38.
After both mixed-case interpretations complete their required preflight, the CLI competitor scheduler must not rerun that same required work under the already-consumed shared deadline. Restrict only the executed follow-up work to optional character classes while retaining the full admitted frontier for cumulative disclosure accounting. This preserves a required-pass candidate if optional work reaches the deadline and keeps the public capture-mass calculation unchanged. The regression asserts that both full mixed-case follow-up searches enter optional-only mode. Refs #37.
Gate restore and existing-seed wallet initialization on the independently recorded BIP32 master fingerprint before any Bitcoin Core wallet mutation. Keep the correction path from disclosing or reusing a fingerprint derived from the candidate being authenticated. Fixes #30.
Every correction plan returned its target set, that same set as primary, an empty reduced set, and a true timed flag. Only the targets and primary set were consumed. Derive primary from targets at the call site and remove the other fields. The search engine also accepted reduced without reading it, so remove that argument and update its test and benchmark callers. Search order and capture accounting remain unchanged. Refs #46.
The preceding all-isinstance check rejects every non-share, so the list-comprehension predicate in recovery could never discard an item. Pass the validated list directly, using a type cast to express the established invariant to mypy. Recovery still copies and validates the sequence internally. Refs #46.
When RIPEMD-160 is unavailable, a valid standard Bails identifier cannot be checked. Preserve the Bails-alpha SHA-256 result and distinguish that inconclusive state from a completed identifier mismatch, so the no-record restore prompt does not claim the cards are wrong. Keep the independent fingerprint and explicit operator-confirmation boundary unchanged. Refs #79
The no-record restore flow can no longer claim a standard Bails identifier mismatch when RIPEMD-160 is unavailable. Record that platform-dependent inconclusive outcome in both the security model and invariant so reviewers can distinguish it from a completed comparison. Refs #79
An existing hex seed or codex32 master secret previously reached the wallet-record fingerprint check only after new recovery cards had been generated and confirmed. Check the typed record immediately after parsing the source, before any card output or ceremony. Preserve the explicit recordless path at the same early decision point, and pass the checked result through to wallet initialization so it is not prompted twice. Cover matching, mismatching, and recordless flows for both source encodings. Refs #30.
Translate Ctrl-C or EOF at the early wallet-record gate for ms32 create --existing into the existing wallet-setup interruption path. This keeps an operator from being told to invalidate a pre-existing recovery card before any new share ceremony has started. Add a focused regression proving the interruption occurs before share creation or output and preserves the valid-backup message.
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
A raw seed imported with create --existing was assigned a temporary random identifier for the no-record safety screen, then assigned a different random identifier when the new share set was created. Reuse the first identifier as the share-set identifier so the safety screen describes the backup that will actually be produced.\n\nExtend the recordless-creation regression to require the displayed, emitted, and imported identifiers to agree.\n\nRefs #30
Entering a complete valid secret during interactive share recovery intentionally supersedes the partial share set. Previously that mode switch happened silently, which made correct behavior look like discarded input. Emit one explicit notice only when shares were already accepted, and pin the behavior in the existing interactive recovery regression. Refs #38
cebecfc to
4ea72bb
Compare
The package-level core_descriptors adapter exposes import-record construction that runtime callers no longer need. Bitcoin Core already owns public derivation, while private descriptor construction is only an implementation detail of the Core adapter. Remove the unused public-deriver protocol and branch, keep the record builder private, and make installed/public-API checks enforce that boundary. Internal tests and verification tools continue to exercise the same fixed descriptor templates and arbitrary account handling. Fixes #63
43af20d to
eb29499
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
4ea72bb to
3d8510a
Compare
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head re-review: ACK eb29499.
The current change removes the obsolete supported core_descriptors API and public-deriver branch while preserving #7's Core-native account-0 initialization and master_xprv. Package-level __all__ is reduced to the intended 23 names; the historical pre-#7 Core import-record hunk is absent. There are no inline review threads. Exact-head Python-package run 633 and Bitcoin Core wallet-fixture run 27 both succeeded.
No code blocker found. The source change remains a focused human-authored commit; the mechanical refresh still requires normal human integration review.
What
Remove the obsolete supported Python API for constructing Bitcoin Core descriptor records:
core_descriptorsfrom package-levelcodex32.__all__and supported API documentation;wallet.py;Why
Current
reviewability-v1exportscore_descriptorseven though #7 moved wallet initialization to Bitcoin Core nativeaddhdkey/createwalletdescriptorsetup. External callers no longer need an import-record construction API or the obsoleteWalletPublicDeriverabstraction.master_xprvremains a separately reviewed supported primitive.The base has 24 package-level exports while
docs/developer/api.mdstill says 25. Removingcore_descriptorsmakes package__all__23 names. #53 publishes reference-vector helpers at their owning modules and deliberately does not add them to package-levelcodex32.__all__.Current head
eb29499is the single original human-authored change replayed onto current #95 (4ea72bb) after the refreshed restore stack/. The refresh deliberately drops the obsolete pre-#7_bitcoin_core.pyimport-descriptor/account-7 hunk and keeps #7-deleted legacy tools deleted. Production_bitcoin_core.pyis unchanged by this PR; v1 keeps Core-native account-0 wallet setup. #68 tracks future nonzero accounts when Core exposes the needed selector.Validation
python -O: 64 passed;compileallandgit diff --check: pass;__all__: 23 names at this head;A disposable full local run stopped only because that ambient Python environment lacks the dev-only
hypothesispackage; the focused tests above do not require it. The repository CI installs the declared development test dependencies and is the authoritative full-matrix check.Fixes #63.
AI assistance was used to mechanically refresh and validate this user-authorized branch-to-branch contribution; the source change remains one human-authored commit.